Why some metrics have blank lines in between but others don't? I think we can remove all these blank lines to be compact.

Should this be std::shared_ptr<Expression>?

This hard-codes our reported duration unit to be milliseconds, which violates the spec I think?

We are missing some fields like projectedFieldIds and projectedFieldNames from the Java implementation. I think they are required by the REST spec: https://github.com/apache/iceberg/blob/149cc464f9b7df800cc5718af725983473819504/open-api/rest-catalog-open-api.yaml#L3990-L4023

Please use kInvalidSnapshotId defined in the iceberg/constant.h

The comment is inconsistent.

ditto

Should we remove this as this is not defined by the Java implementation?

If we define MetricsReport as a std::variant, we cannot support customizing metrics report. For example, engines may have more metrics to report than defined by the Java implementation. Even the REST spec does not explicitly define what keys are required.

Instead, should we define the MetricsReport like below?

struct MetricsReport {
  std::string kind;  // can be "scan" or "commit", or whatever customized
  std::unordered_map<std::string, CounterResult> counter_results;
  std::unordered_map<std::string, TimerResult> timer_results;
};

What do you think?

After thinking more on this, I'm fine to use your current approach to define ScanReport and CommitReport with explicit fields. MetricsReports are collected by this library so users do not have the flexibility to customize them. We only need to customize MetricsReporter.

I like the bag of keys approach because it provides for customization without necessarily having to require compilation. let me think a bit more about the entire thing, I might reach out on slack for a quick sync.

How do we support the Java parity CompositeMetricsReporter? It would be useful if we want to report metrics to multiple sinks.

Missing meson.build equivalent for this subdirectory

Transaction::Commit() always calls ReportCommitMetrics() after a successful metadata commit, even for metadata-only transactions. That helper then reads current_snapshot() from the post-commit table and reports it as if this transaction created that snapshot, which can re-emit stale snapshot metrics and a misleading operation name. Java only reports commit metrics from snapshot-producing commits (CreateSnapshotEvent in SnapshotProducer), so the reporting hook should move closer to snapshot-producing updates or otherwise prove that this transaction actually created a new snapshot before emitting a CommitReport.

The REST catalog now loads a local reporter from catalog properties, but it never constructs the built-in REST reporter for the /metrics endpoint and never combines the two. As a result, scan/commit reports from tables loaded through RestCatalog are never POSTed back to the server even when the server advertises ReportMetrics support. Java's RESTSessionCatalog explicitly combines the catalog reporter with RESTMetricsReporter, so this leaves the feature only partially implemented on the C++ side.

This loader treats metrics-reporter-impl as a lowercase registry key and defaults to noop, while Java and the Iceberg docs treat the same property as a fully qualified class name whose instance is initialized with catalog properties. On top of that, the catalog constructors ignore Load() errors and Table falls back to noop, so a Java-style config string or typo quietly disables metrics instead of surfacing a configuration error. That is a public-contract drift, not just an implementation detail.

while Java and the Iceberg docs treat the same property as a fully qualified class name whose instance is initialized with catalog properties

the divergence is because c++ does not support reflection. I decided on making things local case to reduce the risk of typos based on caps on/off.

typo quietly disables metrics instead of surfacing a configuration error
I will update to surface a configuration error.

ScanReport embeds a plain ScanMetrics struct of raw integers and std::chrono::nanoseconds, but Java's contract is layered: live counters/timers are collected in ScanMetrics, converted into ScanMetricsResult, and then wrapped by ScanReport. That layering preserves units, optional presence, noop semantics, and serialization behavior. In C++, every metric defaults to 0, so consumers cannot distinguish "not collected" from a real zero value, and the report shape is no longer compatible with Java's richer contract.

CommitMetrics exposes replaced_manifest_count, but ReportCommitMetrics() never reads SnapshotSummaryFields::kManifestsReplaced into it. Java's CommitMetricsResult.from(...) does populate the corresponding manifestsReplaced field from the snapshot summary. Any consumer relying on this report will currently observe 0 even when the commit actually replaced manifests.

The scan report counts equality and positional delete files, but it never increments indexed_delete_files and it never separates deletion vectors from ordinary positional deletes. Java's ScanMetricsUtil.indexedDeleteFile(...) increments all three counters (indexedDeleteFiles, positionalDeleteFiles/dvs, equalityDeleteFiles). Because the C++ report schema already exposes indexed_delete_files and dvs, leaving them unset produces parity drift and misleading metrics for DV-heavy tables.

CommitReport only carries summary-derived counters, but Java's commit reporting model also has live commit metrics (totalDuration, attempts) collected through CommitMetrics and serialized through CommitMetricsResult. Because those fields are absent from the C++ public model entirely, even a future implementation cannot fully mirror Java's commit report without another API change.

The builder stores a reporter internally, but there is no public scan-level API to add an extra reporter the way Java's Scan.metricsReporter(...) does. That means C++ can only use the reporter injected from the table/catalog path, while Java supports composing an additional reporter for a particular scan. For observability integrations and tests, that is a meaningful flexibility gap.

good catch will address.